-
Notifications
You must be signed in to change notification settings - Fork 645
add initial cargo ci #3409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
add initial cargo ci #3409
Conversation
|
The tests are failing. I think several of them just need a |
| run!("for p in \"$GITHUB_WORKSPACE\" \"${RUNNER_TEMP:-/__t}\" \"${RUNNER_TOOL_CACHE:-/__t}\"; do [ -d \"$p\" ] && setfacl -R -m u:ue4:rwX -m d:u:ue4:rwX \"$p\" || true; done")?; | ||
|
|
||
| run!("export CARGO_HOME=\"${RUNNER_TOOL_CACHE:-/__t}/cargo\"")?; | ||
| run!("export RUSTUP_HOME=\"${RUNNER_TOOL_CACHE:-/__t}/rustup\"")?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I think a lot of this doesn't apply to users running these tests locally
|
|
||
| Some(CiCmd::CliDocs) => { | ||
| run!("pnpm install --recursive")?; | ||
| run!("cargo run --features markdown-docs -p spacetimedb-cli > docs/docs/cli-reference.md")?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that this assumes we're running from the root directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does. Do you think this it a problem to assume that? If so, would it be a good idea to try to figure out where in the dir tree the command was ran (maybe looking upwards for the root Cargo.toml or something?) to adjust accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might just add a check similar to what tools/upgrade-version does:
tools/upgrade-version/src/main.rs- let current_dir = env::current_dir().expect("No current directory!");
tools/upgrade-version/src/main.rs- let dir_name = current_dir.file_name().expect("No current directory!");
tools/upgrade-version/src/main.rs- if dir_name != "SpacetimeDB" && dir_name != "public" {
tools/upgrade-version/src/main.rs: anyhow::bail!("You must execute this binary from inside of the SpacetimeDB directory, or use --spacetime-path");
(alternatively we could use git rev-parse --show-toplevel and then just go to that directory)
| run: cargo ci self-docs --check | ||
|
|
||
| cli_docs: | ||
| name: Check CLI docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we want to replace this one with a cargo ci invocation as well?
| @@ -0,0 +1,4 @@ | |||
| #[allow(clippy::disallowed_macros)] | |||
| fn main() { | |||
| println!("cargo:rustc-env=TARGET={}", std::env::var("TARGET").unwrap_or_default()); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this part for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this allows for the default parameter for target on the update-flow. Mostly for running it locally without needing to specify that flag manually.
SpacetimeDB/tools/ci/src/main.rs
Line 177 in 1c0406d
| let target = target.unwrap_or_else(|| env!("TARGET").to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah gotcha. in that case, nit: we could either use the variables that are provided already:
const ARCH: &str = env!("CARGO_CFG_TARGET_ARCH");
const OS: &str = env!("CARGO_CFG_TARGET_OS");
const ENV: &str = env!("CARGO_CFG_TARGET_ENV");
or we could only pass --target {target} to cargo build if it's provided by the user.
I kind of prefer that last option. It seems intuitive still.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(honestly I'm not 100% sure that we even need the target param.. I think the default on each platform might be the one we want anyway.. but we should keep it for now just for parity with the original CI).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that --target is optional, is it reasonable to remove this?
| let mut env = env::vars().collect::<HashMap<_, _>>(); | ||
| env.extend(additional_env.iter().map(|(k, v)| (k.to_string(), v.to_string()))); | ||
| log::debug!("$ {cmdline}"); | ||
| let status = cmd!("bash", "-lc", cmdline).full_env(env).run()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I'm not 100% sure how this will work on windows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to work this out, actually. Because all other commands are also unix-like. Do you have an idea on how to make it work on windows too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we ask @jdetter to try running some of cargo ci on his windows machine. If it "just works" then great. If it doesn't due to bash missing, then I think we might need to make a separate ticket for making all of our CI properly runnable on windows. I suspect it might work properly in the GitHub workflow as-is though, because I believe that will have bash installed.
|
|
||
| use crate::Cli; | ||
|
|
||
| pub fn generate_cli_docs() -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: any chance we could use clap_markdown like we do for the SpacetimeDB CLI docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed. I didn't notice that we had that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is already working, we could add a TODO and just leave it for a future PR.
|
FYI I pushed a merge commit so the CI would run again |
| - name: Run smoketests | ||
| # Note: clear_database and replication only work in private | ||
| run: python -m smoketests ${{ matrix.smoketest_args }} -x clear_database replication | ||
| run: cargo ci smoketests -- ${{ matrix.smoketest_args }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
little nit: I kinda preferred when the skipped tests were provided here, since they're going to be skipped on every platform. Not super important, but also not super important to change (although we could consider renaming smoketests_args to extra_smoketest_args to be clearer).
If you prefer it this way, that's fine with me.
| @@ -162,19 +161,20 @@ fn main() -> Result<()> { | |||
| run!("cargo run -p spacetimedb-cli -- build --project-path modules/module-test")?; | |||
| } | |||
|
|
|||
| Some(CiCmd::Smoketests { args }) => { | |||
| Some(CiCmd::Smoketests { mut args }) => { | |||
| let default_args = ["-x", "clear_database", "replication"]; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I worry about tests being omitted by default. I think the expectation was that there's some easy/intuitive way to run everything.
|
|
||
| Some(CiCmd::CliDocs) => { | ||
| run!("pnpm install --recursive")?; | ||
| run!("cargo run --features markdown-docs -p spacetimedb-cli > docs/docs/cli-reference.md")?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might just add a check similar to what tools/upgrade-version does:
tools/upgrade-version/src/main.rs- let current_dir = env::current_dir().expect("No current directory!");
tools/upgrade-version/src/main.rs- let dir_name = current_dir.file_name().expect("No current directory!");
tools/upgrade-version/src/main.rs- if dir_name != "SpacetimeDB" && dir_name != "public" {
tools/upgrade-version/src/main.rs: anyhow::bail!("You must execute this binary from inside of the SpacetimeDB directory, or use --spacetime-path");
(alternatively we could use git rev-parse --show-toplevel and then just go to that directory)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if we use .map().unwrap_or_default() above, we could use something similar here:
| let github_token_auth_flag = github_token_auth.then_some("--features github-token-auth ").unwrap_or_default(); |
I don't feel strongly one way or another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(or we could have an extra_args string that we append to if target is defined or github_token_auth is true)
| @@ -0,0 +1,4 @@ | |||
| #[allow(clippy::disallowed_macros)] | |||
| fn main() { | |||
| println!("cargo:rustc-env=TARGET={}", std::env::var("TARGET").unwrap_or_default()); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that --target is optional, is it reasonable to remove this?
| let mut env = env::vars().collect::<HashMap<_, _>>(); | ||
| env.extend(additional_env.iter().map(|(k, v)| (k.to_string(), v.to_string()))); | ||
| log::debug!("$ {cmdline}"); | ||
| let status = cmd!("bash", "-lc", cmdline).full_env(env).run()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we ask @jdetter to try running some of cargo ci on his windows machine. If it "just works" then great. If it doesn't due to bash missing, then I think we might need to make a separate ticket for making all of our CI properly runnable on windows. I suspect it might work properly in the GitHub workflow as-is though, because I believe that will have bash installed.
Description of Changes
API and ABI breaking changes
Expected complexity level and risk
Testing